Skip to content

NH-3964 - obsolete Linq.ReflectionHelper in favor of Util.ReflectHelper. #586

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 31, 2017
Merged

NH-3964 - obsolete Linq.ReflectionHelper in favor of Util.ReflectHelper. #586

merged 1 commit into from
Mar 31, 2017

Conversation

fredericDelaporte
Copy link
Member

@fredericDelaporte fredericDelaporte commented Mar 27, 2017 •
edited
Loading

Follow up of NH-3964 and its first PR #574 .

Methods are just moved to ReflectHelper, without changing them.

Trying to use

public static MethodInfo GetMethod<T, TResult>(Func<T, TResult> func, T arg1)
{
return func.Method;
}

is requiring as many overloads as function arguments counts

public static MethodInfo GetMethod<T1, T2, TResult>(Func<T1, T2, TResult> func, T1 arg1, T2 arg2)
{
    return func.Method;
}

Then type inference is sometime inferior:

return GetMethod(Enumerable.GroupBy, (IEnumerable<int>)null, (Func<int, decimal>)null);

Instead of

return GetMethod(() => Enumerable.GroupBy(null, (Func<int, decimal>)null));

And handling instance methods are requiring a Func<Func<>> with a non-null instance:

public static MethodInfo GetMethod<TSource, T1, TResult>(Func<TSource, Func<T1, TResult>> func, TSource source, T1 arg1)
{
	return func(source).Method;
}

GetMethod<string, int, string>(s => s.Substring, "", 10);

So well, even though some "brute force" runtime performance comparison show it is "better", I do not think it is worth all the required changes and additional code for using them.

@hazzik
Copy link
Member

hazzik commented Mar 27, 2017

  • We need about 4 overloads for GetMethodInfo & 4 overloads for GetMethodDefinition, it's true.
  • There are only few cases where we do not have an instance upfront. For string we can use string.Empty and for structs default(T) (where T is a struct). Where we need a real instance we can use the old method.
  • (IEnumerable<int>)null looks clearer as default(IEnumerable<int>)

@fredericDelaporte
Copy link
Member Author

Snapshot dependency failure for failed build: looks like a Team City infrastructure failure, doesn't it? I should probably trigger a later rebuild with a rebase.

fredericDelaporte
fredericDelaporte commented Mar 27, 2017
View reviewed changes
src/NHibernate/Util/ReflectHelper.cs Outdated
}

[Obsolete("Please use Linq.ReflectionHelper instead")]
[Obsolete("Please use Linq.ReflectHelper instead")]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, search&replace does not make it for this obsolete reason. I would fix it tomorrow for triggering another build by the way.

@fredericDelaporte fredericDelaporte force-pushed the NH-3964 branch 2 times, most recently from 0ffd5ea to 0c25d3e Compare March 28, 2017 09:55
@fredericDelaporte
Copy link
Member Author

Amended for updating a doc example referencing ReflectionHelper, and rebased.

@fredericDelaporte
Copy link
Member Author

Found two additional method reflection which are cacheable and were not. Now cached. And rebased.

@hazzik hazzik removed the to review label Mar 30, 2017
hazzik
hazzik approved these changes Mar 30, 2017
View reviewed changes
Copy link
Member

@hazzik hazzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :shipit:

@fredericDelaporte fredericDelaporte merged commit d82d138 into nhibernate:master Mar 31, 2017
@fredericDelaporte fredericDelaporte deleted the NH-3964 branch March 31, 2017 08:49
@fredericDelaporte fredericDelaporte added this to the 5.0.0 milestone Apr 1, 2017
@hazzik hazzik added r: Fixed and removed r: Fixed labels Aug 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hazzik hazzik hazzik approved these changes

Assignees
No one assigned
Labels
r: Fixed t: Improvement
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.
2 participants
@fredericDelaporte @hazzik